Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conditionally include PWACompat #310

Merged
merged 3 commits into from
Jun 2, 2020
Merged

Conditionally include PWACompat #310

merged 3 commits into from
Jun 2, 2020

Conversation

tomayac
Copy link
Contributor

@tomayac tomayac commented Dec 18, 2019

On Apple mobile devices add the proprietary app icon and splashscreen markup. No one should have to do this manually, and eventually this annoyance will go away once https://bugs.webkit.org/show_bug.cgi?id=183937 is fixed.

Fixes #275

@tomayac tomayac added meta Issues involving the build process or the website itself, rather than its content and removed cla: yes labels Dec 18, 2019
@mathiasbynens
Copy link
Member

I'd rather not land workarounds for non-critical engine-specific bugs.

This is an annoying lack of functionality in WebKit that should be fixed upstream, IMHO. Until then v8.dev works fine without.

@tomayac
Copy link
Contributor Author

tomayac commented Dec 18, 2019

I respectfully disagree. It looks super ugly on an iOS/iPadOS home screen and someone went the extra mile to open an issue about it. You (the V8 team) should weigh in on the WebKit bug and ask for it to be fixed. We all agree it's a temporary solution and not our problem, but in this case users should not have to suffer from it.

@tomayac
Copy link
Contributor Author

tomayac commented Dec 18, 2019

CC: @samthor.

@samthor
Copy link

samthor commented Dec 18, 2019

I don't actually have a strong opinion here. PWACompat does its job but it might not be right here.

@tomayac
Copy link
Contributor Author

tomayac commented Dec 18, 2019

Users first.

@RReverser
Copy link
Contributor

RReverser commented May 27, 2020

@tomayac Is this still an issue on Apple devices? If it is, I'd agree that UX is most important and given that this adds overhead only for a specific User-Agent, it should be fine to include.

@tomayac
Copy link
Contributor Author

tomayac commented May 27, 2020

@tomayac Is this still an issue on Apple devices? If it is, I'd agree that UX is most important and given that this adds overhead only for a specific User-Agent, it should be fine to include.

This is still an issue. I have just made the conditional loading even lazier, so that it doesn't load on installed experiences.

@RReverser
Copy link
Contributor

Looking at what it does, do I understand right that we're interested in just <link rel="icon" ...> addition with a different size? If so, maybe it's easier to add the required size to the static markup?

@tomayac
Copy link
Contributor Author

tomayac commented May 27, 2020

The library dynamically (but lazily) creates splash screens(!) and icons for all devices.

@RReverser
Copy link
Contributor

Right, but my question is whether we need all of that to address the specific issue?

@tomayac
Copy link
Contributor Author

tomayac commented May 28, 2020

Right, but my question is whether we need all of that to address the specific issue?

We give people the same experience they get on Android when they install a PWA. Needed? Probably no. Nice to have? Probably yes.

@RReverser
Copy link
Contributor

Right, let me rephrase: are these other features also not working in iOS natively without PWACompat?

@tomayac
Copy link
Contributor Author

tomayac commented May 29, 2020

Right, let me rephrase: are these other features also not working in iOS natively without PWACompat?

This is the list of things the library does. The splash screens are a nice touch. Right now minimal-ui isn't supported on iOS, so it runs in a regular browser window (which doesn't trigger splash screens to show).

image

@RReverser
Copy link
Contributor

Sounds reasonable; one more question: it seems like the meta tag for the splash screen should always be present in the app to work, no?

If so, we probably should revert the last change so that PWACompat is included even for installed experiences.

@tomayac
Copy link
Contributor Author

tomayac commented May 29, 2020

Sounds reasonable; one more question: it seems like the meta tag for the splash screen should always be present in the app to work, no?

The splash screens will be dynamically created based on the Web App Manifest values. No meta tag needed.

If so, we probably should revert the last change so that PWACompat is included even for installed experiences.

When navigator.standalone evaluates to true, the library does nothing. Since right now this is false since we run in a browser tab, the library will be included. If we ever decide to run standalone (which iOS supports), we don't have to think of changing the code.

@RReverser
Copy link
Contributor

So you're saying that this latest added condition is not actually ever triggered? I'd suggest to just remove it for now then. If / when we decide to run in standalone, we can revisit the code (and see if Apple fixed their side by then).

@tomayac
Copy link
Contributor Author

tomayac commented Jun 2, 2020

So you're saying that this latest added condition is not actually ever triggered? I'd suggest to just remove it for now then. If / when we decide to run in standalone, we can revisit the code (and see if Apple fixed their side by then).

Done.

@RReverser RReverser merged commit df5058a into master Jun 2, 2020
@RReverser RReverser deleted the issue-275 branch June 2, 2020 11:22
@mathiasbynens
Copy link
Member

@tomayac […] given that this adds overhead only for a specific User-Agent, it should be fine to include.

Unfortunately, it doesn’t just add overhead for Safari. Check the DevTools → Network tab in Chrome, for example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes meta Issues involving the build process or the website itself, rather than its content
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Why I can't add PWA to my phone in Safari 13
5 participants